Can one of the admins verify this patch?

Could someone say "ok to test"? ;)

Ok to test

Can we look into using PackageProject here?

Can we look into using PackageProject here?

I looked at it. It doesn't quite work, once Thrust is added, since it assumes it can just do a bunch of find_dependency(...) calls, but we need to have a thrust_create_target(), too. I don't think it has a way to add cmake_minimum_required() into the config file, either, which is advisable since we're relying on find_package(CUDAToolkit), which is relatively recent.

We could possibly submit PRs to PackageProject to make it more customizable so that it could fit the needs, but in the end I'm not sure we end up with something simpler.

Oops, I just found this comment I wrote in github a couple of days ago, but apparently never actually submitted.
This PR should not be merged before settling this.

So it's time to talk about the caveats.

First of all, the potential for breaking something that currently works is fairly low. Essentially, this PR only adds some new cmake config files that are installed alongside rmm's headers.

But there are some complications related to thrust:

Currently, the installed target does depend on spdlog::spdlog_header_only, CUDA::cudart and rmm::Thrust. The first two are pulled in automatically when doing find_package(rmm), but not yet rmm::Thrust.

So that's why it's WIP, it's only actually usable by a downstream project if the downstream project hacks around this limitation.

I think there are two options to handle this:

1. drop the explicit dependency on rmm::Thrust for now.
2. pull in Thrust 1.10.0 via find_dependency.

Option 1 would make things work without trouble for the typical downstream project. They'd likely end up using the thrust installed with their CUDA toolkit, which is probably good enough for them. If they do care about a newer thrust, they'd pull that in explicitly, and it'd get used.

Option 2 is the more correct way of doing things, but it also makes it harder to actually use rmm via find_package(rmm). There are a number of reasons:

• Currently, thrust installs its config in a non-standard place, where it won't be found using the standard methods like CMAKE_PREFIX_PATH (thrust-config.cmake is installed in a non-standard location NVIDIA/thrust#1292). Also, lots of pre-installed thrust's don't have a cmake installed with them in the first place (CUDA Toolkit packaging does not include Thrust's CMake find_package config files NVIDIA/thrust#1294)
• thrust, when used via add_subdirectory()/CPMAddPackage won't install itself, PR pending (cmake: option to install thrust when used via add_subdirectory() NVIDIA/thrust#1297).
• In order to use the development version of thrust, once the above is merged, one might also hit a version mismatch, PR pending (cmake: relax package compatibility constraints NVIDIA/thrust#1298).

I'm kinda ambivalent about what the best way to proceed is. One could choose option 1 for now and switch to doing things properly once a new version of thrust is released (or once the PRs above are merged, depend on a specific version from thrust's main branch). Or do option 2 correctly already, which means that things will only really become usable as thrust gets updated.

FWIW, this is the diff for option 2:

diff --git a/cmake/RMM_thirdparty.cmake b/cmake/RMM_thirdparty.cmake
index 319d031..ab5abbc 100644
--- a/cmake/RMM_thirdparty.cmake
+++ b/cmake/RMM_thirdparty.cmake
@@ -23,6 +23,10 @@ CPMFindPackage(
   GIT_TAG 1.10.0
   VERSION 1.10.0
   GIT_SHALLOW TRUE
+  OPTIONS
+    # If there is no pre-installed thrust we can use, we'll install our fetched copy
+    # together with RMM
+    "THRUST_INSTALL TRUE"
   )

 thrust_create_target(rmm::Thrust FROM_OPTIONS)
diff --git a/rmm-config.cmake.in b/rmm-config.cmake.in
index a720acf..617828f 100644
--- a/rmm-config.cmake.in
+++ b/rmm-config.cmake.in
@@ -6,6 +6,8 @@ cmake_minimum_required(VERSION 3.17)
 include(CMakeFindDependencyMacro)
 find_dependency(CUDAToolkit)
 find_dependency(spdlog 1.7.0)
+find_dependency(Thrust 1.10.0)
+thrust_create_target(rmm::Thrust FROM_OPTIONS)

 include("${CMAKE_CURRENT_LIST_DIR}/rmm-targets.cmake")
 include("${CMAKE_CURRENT_LIST_DIR}/rmm-config-version.cmake")

Option 1 would be simply going to back to using $<BUILD_INTERFACE:...> for the dependency on rmm::Thrust, so that it doesn't get recorded in the installed package.

@harrism, I committed all of your suggested doc changes, btw -- thanks!

Just FYI, I think RMM should work fine with thrust 1.9.x. HOWEVER, libcudf needs Thrust 1.10+ I think and the most common use cases of RMM also use cuDF.

So given the timeframe for the next release, I'm inclined to address the last little hitch that I'm aware of in this PR as well (besides finishing the thrust issue):

Right now, RMM does this:

if(USE_NVTX)
  message(STATUS "Using Nvidia Tools Extension")
else()
  target_compile_definitions(rmm INTERFACE NVTX_DISABLE)
endif(USE_NVTX)

By default, USE_NVTX is off, which adds -DNVTX_DISABLE to the flags, and as such, the installed cmake configuration picks that up and hence a downstream project using RMM may unexpectedly end up with its NVTX disabled.

So this flag is something that maybe should be set for the benchmarks, but not generally for RMM and all of its consumers. Right now, it really doesn't need to be set at all, since I believe no nvtx is used anywhere at all.

An easy solution would be to get rid of the above, since it isn't even being used. That would also allow to reintroduce it later as RMM_USE_NVTX, if desired. But I'm not sure it's really clear at this point in which fashion nvtx would best be handled, anyway.

You can remove the NVTX stuff. I have an open PR that I plan to update in the next release to add some NVTX regions and I can add a new flag there if necessary.

Note that while burndown means "no new PRs", we still have about a week during burndown to finish in progress PRs.

Note that while burndown means "no new PRs", we still have about a week during burndown to finish in progress PRs.

Yeah, I understand, that's why I didn't want to come with a little follow-up PR later. The latest push now removes USE_NVTX for the time being, and it also uses variables for the versions of thirdparty dependencies (thrust and spdlog).

There's some asymmetry in that we required Thrust >= 1.10.0 at build time (and fetch if necessary), but accept Thrust >= 1.9.0 when using the installed RMM. I think that's reasonable for the time being, though. I didn't leave a warning if Thrust < 1.10 ends up being used, but I can easily add one back if desired. When cmake finds an old Thrust version it will show what version it is, and its location, so it's at least somewhat visible in any case:

-- Found Thrust: /opt/spack/opt/spack/linux-ubuntu18.04-haswell/gcc-7.5.0/cuda-10.2.89-cdzvh6elxmxj3kela4cnnvrzpjy2ett4/include (found suitable version "1.9.7", minimum required is "1.9.0")

RMM will prefer a newer Thrust, found by config mode, though, before falling back to the old way.

There's a bit of a similar issue to USE_NVTX with PER_THREAD_DEFAULT_STREAM. But at least it's off by default, so it won't usually interfere. The nice thing about a header-only library is that one doesn't have to make these choices at build time for the library, hence it also would be better it they were not made (and recorded in the config). I think it'd better to just turn PTDS on (optionally) for the tests/benchmarks. (That also means one could have the tests compiled twice, with and w/o, where one wants to test both cases.)

I just noticed that in an earlier commit you modified FindThrust.cmake, rather than just pulling it in without changes. That changes the process. I would have to start over with a much more involved legal process. Any chance of using the original unmodified FindThrust? (Or getting Thrust to provide their own?) Or somehow not needing FindThrust? (We didn't need it previously...)

I just noticed that in an earlier commit you modified FindThrust.cmake, rather than just pulling it in without changes. That changes the process. I would have to start over with a much more involved legal process. Any chance of using the original unmodified FindThrust? (Or getting Thrust to provide their own?)

I started over and implemented a FindThrust.cmake from scratch. This at least has the advantage that I could actually make it provide compatible behavior to the configs installed by thrust since 1.9.10, which simplifies the rmm-config.cmake quite a bit. The only three lines of code that aren't my own are from https://github.com/NVIDIA/thrust/blob/main/thrust/cmake/README.md, which mirror the version calculations done in thrust/version.h.

Or somehow not needing FindThrust? (We didn't need it previously...)

Well, a prior revision of this PR was like that, if a newer Thrust wasn't found by config, it'd just keep its fingers crossed that some other thrust would be found automatically at build time (which would usually work, since it's in the cuda toolkit). But I think what's there now is cleaner, and at some point in the future, when Thrust that comes with its own cmake config is commonly available, the FindThrust.cmake can just be removed and things will still work.

It's worth noting that this FindThrust.cmake is deliberately put into cmake/install/, where it won't be found or used during the RMM build at all. The only reason it's there is so that it's installed together with the rest of RMM, so that a downstream cmake project that uses RMM handles RMM's thrust dependency properly. This PR, for the most part adds a new feature (ie., some additional files get installed for downstream use) and keeps existing use almost untouched.

This is fantastic. Thanks very much for doing this. It's not that we couldn't use the VTK-m code, its just that I have to go through a different process with the lawyers if we plan to modify it.

BTW, if you ever want to discuss anything not directly related to a PR off of github, you can find us on the RAPIDS public slack linked from this page: https://rapids.ai/community.html

@kkraus14 you might want to have a look before we merge?